Additional unit tests for IOUtil #1149

kachulis · 2018-06-15T14:55:40Z

Description

Additional unit test to IOUtilTest to increase coverage of IOUtil.

Checklist

[x ] Code compiles correctly
New tests covering changes and new functionality
[x ] All tests passing
[NA] Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

codecov-io · 2018-06-15T15:10:19Z

Codecov Report

Merging #1149 into master will increase coverage by 0.174%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##              master    #1149       +/-   ##
==============================================
+ Coverage     68.156%   68.33%   +0.174%     
- Complexity      7962     8001       +39     
==============================================
  Files            540      541        +1     
  Lines          32622    32681       +59     
  Branches        5533     5536        +3     
==============================================
+ Hits           22234    22331       +97     
+ Misses          8169     8124       -45     
- Partials        2219     2226        +7

Impacted Files	Coverage Δ	Complexity Δ
...mtools/reference/ReferenceSequenceFileFactory.java	`78.723% <0%> (-5.06%)`	`22% <0%> (+3%)`
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
.../samtools/reference/AbstractFastaSequenceFile.java	`52.273% <0%> (-2.727%)`	`9% <0%> (+1%)`
...ain/java/htsjdk/samtools/SAMFileWriterFactory.java	`62.162% <0%> (-1.976%)`	`50% <0%> (+3%)`
...kablestream/ReadableSeekableStreamByteChannel.java	`72.222% <0%> (ø)`	`5% <0%> (?)`
src/main/java/htsjdk/samtools/CRAMFileReader.java	`74.5% <0%> (+0.258%)`	`53% <0%> (+4%)`	⬆️
...ls/reference/AbstractIndexedFastaSequenceFile.java	`75.325% <0%> (+1.352%)`	`18% <0%> (+1%)`	⬆️
.../htsjdk/samtools/reference/FastaSequenceIndex.java	`61.972% <0%> (+1.678%)`	`12% <0%> (ø)`	⬇️
...a/htsjdk/samtools/reference/FastaSequenceFile.java	`78.049% <0%> (+2.334%)`	`22% <0%> (+1%)`	⬆️
...dk/samtools/seekablestream/SeekableFileStream.java	`73.81% <0%> (+2.381%)`	`16% <0%> (+1%)`	⬆️
... and 3 more

yfarjoun

Welcome to htsjdk and thanks for submitting these tests.

yfarjoun · 2018-06-15T15:56:27Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

@@ -520,4 +527,253 @@ public void testIsBlockCompressedOnJimfs(Path file, boolean checkExtension, bool
             Assert.assertEquals(IOUtil.isBlockCompressed(jimfsFile, checkExtension), expected);
         }
    }
+    @DataProvider
+    public static Object [][] filesToCompress(){
+        return new Object[][]{


IntelliJ has a "Reformat Code" option which will clean up a lot of the stylistic issues that I might list here:

fewer empty lines

space between ']' and '('

space after ','

space between ')' and '{'

no newline between ')' and '{'

space around operators

space between if/while/for and '('

no space between method-name and '('

if/else with brackets and the else line looking like '} else {'
-make final any variable that can be (method arguments included)

yfarjoun · 2018-06-15T16:00:11Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+    public void testCompressionLevel(Path file,String extension,int lastDifference) throws IOException{
+        long origSize=file.toFile().length();
+        long previousSize=origSize;
+        String tmpPath = System.getProperty("java.io.tmpdir");


use File.createTempFile for creating temporary files. (here and below)

yfarjoun · 2018-06-15T16:00:49Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        File tmpDir = new File(tmpPath, userName);
+        tmpDir.mkdir();
+        tmpDir.deleteOnExit();
+        for(int cl=1;cl<=9;cl++)


use meaningful variable names

yfarjoun · 2018-06-15T16:01:58Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+            OutputStream outStream=IOUtil.openFileForWriting(outFile.toFile());
+            IOUtil.transferByStream(inStream,outStream,origSize);
+            outStream.close();
+            outFile.toFile().deleteOnExit();


put deleteOnExit() call right after file "creation"

yfarjoun · 2018-06-15T16:02:14Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+            InputStream inStream=IOUtil.openFileForReading(file);
+            OutputStream outStream=IOUtil.openFileForWriting(outFile.toFile());
+            IOUtil.transferByStream(inStream,outStream,origSize);
+            outStream.close();


for resources that need to be closed, use try-with-resources

yfarjoun · 2018-06-15T16:03:06Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+            IOUtil.transferByStream(inStream,outStream,origSize);
+            outStream.close();
+            outFile.toFile().deleteOnExit();
+            long newSize=outFile.toFile().length();


use Files.size(Path)

yfarjoun · 2018-06-15T16:06:01Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+
+    }
+
+    @Test(dataProvider = "badCompressionLevels")


use "expectedException" on the test here.

yfarjoun · 2018-06-15T16:11:44Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        tmpDir.mkdir();
+        tmpDir.deleteOnExit();
+        Path outFile=Paths.get(tmpPath,userName,"tmp"+file.getFileName());
+        ProcessExecutor.execute(new String[]{"touch", outFile.toAbsolutePath().toString()});


I think it would be better to open and close the file than to spawn an external command here.

yfarjoun · 2018-06-15T16:28:14Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        tmpDir.mkdir();
+        tmpDir.deleteOnExit();
+        Path file = Paths.get(tmpDir.getAbsolutePath(), "tmp.txt");
+        Random rand = new Random(System.currentTimeMillis());


Better to have no true randomness in code...use a fixed seed here.

kachulis · 2018-06-15T20:47:38Z

Thanks Yossi. I have updated to address your requests.

yfarjoun

looking better!
some more comments.

yfarjoun · 2018-06-18T18:06:09Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+                {TEST_DATA_DIR.resolve("words_longs.txt"), ".bfq", 8},
+                {TEST_VARIANT_DIR.resolve("test1.vcf"), ".gz", 7},
+                {TEST_VARIANT_DIR.resolve("test1.vcf"), ".bfq", 7}
+


still too many newlines here and other places.

yfarjoun · 2018-06-18T18:29:57Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        IOUtil.copyFile(file.toFile(), outFile.toFile());
+
+
+        final Stream<String> stream = Files.lines(file);


given that we do not need to save on memory here,
you can replace the remaining lines of code here with a

Assert.assertEquals(File.lines(file),Files.lines(outFile))

yfarjoun · 2018-06-18T18:30:56Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        try {
+            IOUtil.copyFile(file.toFile(), outFile.toFile());
+        } catch (SAMException e) {
+            file.toFile().setReadable(true);


why change the readability status here?

if this is needed then put a comment.

you want a finally here not a catch.

yup, you're right should be a finally. For reference, need to change back to readable so that other tests which need to read this file later will not fail (along with just being best to leave things as they were before the test). adding explanation as comment in code.

yfarjoun · 2018-06-18T18:42:54Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+            if (listExpected.contains(name)) {
+                expectedFiles.add(file.toFile());
+            }
+            file.toFile().deleteOnExit();


move this up to be right after the resolve call.

yfarjoun · 2018-06-18T18:44:19Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+                writer.newLine();
+            }
+        }
+        final IterableOnceIterator<String> retLines = IOUtil.readLines(file.toFile());


again, this whole block can be replaced with a single assert, I think.

yfarjoun · 2018-06-18T18:44:53Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        final Stream<Path> fileStream = Files.walk(TEST_VARIANT_DIR);
+        final Stream<Path> copyFileStream = Files.walk(copyToDir);
+
+        final Iterator<Path> itFileStream = fileStream.iterator();


inline stream and iterator

you can simplify this by actually using the stream...

final List<Path> collect = Files.walk(TEST_VARIANT_DIR).filter(f -> !f.equals(TEST_VARIANT_DIR)).collect(Collectors.toList());

and then compare with Assert.assertEquals()

lbergelson

A 2mb file of random words is a very large test file for htsjdk. Would it be possible to reduce he file size? One easy thing to do would be to zip it in the repo and unzip it for the test. Another thing might be to generate it from a smaller underlying dictionary instead.

…emporary long word file for compression testing in IOUtilTest

kachulis · 2018-06-19T15:40:27Z

Sure. I went the dictionary route, so the 2mb random word file is replaced with a ~50kb dictionary which is used to create a temporary few mb random word file (which gets deleted on exit) instead.

lbergelson · 2018-06-19T15:46:02Z

@kachulis Thanks. I appreciate the extra effort. One 2mb isn't that big of a deal, but if we add a lot of them we end up with a huge unwieldy repo so it's always good to keep things slim.

kachulis · 2018-06-20T12:56:20Z

@lbergelson no problem, that makes sense

yfarjoun

A few last comments.

This is looking good! thanks!

yfarjoun · 2018-06-20T14:38:36Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        //build long file of random words for compression testing
+        WORDS_LONG = Files.createTempFile("words_long", ".txt");
+        WORDS_LONG.toFile().deleteOnExit();
+        List<String> wordsList = new ArrayList<String>();


can be final

yfarjoun · 2018-06-20T14:41:56Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+                {-1},
+                {10}
+        };
+


straggler unneeded newline.

yfarjoun · 2018-06-20T14:45:32Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        final Random rand = new Random(seed);
+        final int nLines = 5;
+        final List<String> lines = new ArrayList<String>();
+        try (final BufferedWriter writer = Files.newBufferedWriter(file)) {


Not sure it's nicer, but you can wrap the bufferedWriter with a PrintWriter and then call println() instead of write() + newLine().

yfarjoun · 2018-06-20T14:48:15Z

src/test/java/htsjdk/samtools/util/IOUtilTest.java

+        final Path copyToDir = Files.createTempDirectory("copyToDir");
+        copyToDir.toFile().deleteOnExit();
+        IOUtil.copyDirectoryTree(TEST_VARIANT_DIR.toFile(), copyToDir.toFile());
+        final List<String> collect = Files.walk(TEST_VARIANT_DIR).filter(f -> !f.equals(TEST_VARIANT_DIR)).map(p -> p.getFileName().toString()).collect(Collectors.toList());


is toString needed after getFileName?

No, you're right, can make this a List<Path> instead of a List<String> and still works.

I updated your comment since github sanitizes things that look like html....

ha, oops, thanks

yfarjoun

Thanks!

Additional unit tests for IOUtil

ab3899c

yfarjoun requested changes Jun 15, 2018

View reviewed changes

cleaning things up as requested by Yossi

7b58b76

yfarjoun requested changes Jun 18, 2018

View reviewed changes

more cleanup of IOUtilTest

859863a

lbergelson reviewed Jun 18, 2018

View reviewed changes

replacing words_long.txt with much smaller dictionary used to build t…

7818cbf

…emporary long word file for compression testing in IOUtilTest

yfarjoun requested changes Jun 20, 2018

View reviewed changes

few more cleanups

d1030b4

yfarjoun approved these changes Jun 21, 2018

View reviewed changes

yfarjoun merged commit 38a24d5 into samtools:master Jun 21, 2018

		IOUtil.copyFile(file.toFile(), outFile.toFile());


		final Stream<String> stream = Files.lines(file);

Additional unit tests for IOUtil #1149

Additional unit tests for IOUtil #1149

Conversation

kachulis commented Jun 15, 2018

Description

Checklist

codecov-io commented Jun 15, 2018 • edited Loading

Codecov Report

yfarjoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachulis commented Jun 15, 2018

yfarjoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

kachulis commented Jun 19, 2018

lbergelson commented Jun 19, 2018

kachulis commented Jun 20, 2018

yfarjoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachulis Jun 20, 2018 • edited by yfarjoun Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 15, 2018 •

edited

Loading

yfarjoun Jun 18, 2018 •

edited

Loading

kachulis Jun 20, 2018 •

edited by yfarjoun

Loading